Add pagination in the stock_movement end-point #6960

mbayopanda · 2023-03-06T12:26:22Z

This PR adds paging in the stock_movment end-point.

To perform queries with paging, you have to add: ?paging=true in the HTTP request, which returns an object with two elements:

{
  rows: [], // records
  pager: {
    "total": 18, // total of records found in the database
    "page": 4, // the current page, pages are indexed from 1
    "page_size": 5, // the number of records returned by pages
    "page_min": 15, // the minimum counted record of the page
    "page_max": 20, // the maximum counted record of the page
    "page_count": 4 // the total of pages
  } // pagination information
}

To test the feature:

Log into BHIMA (with browser or REST Tools)
Go to this route: stock/lots/movements/?paging=true to have pager information
Go to this route: stock/lots/movements/?paging=true&page=2 to have page 2 of records
Go to this route: stock/lots/movements/?paging=true&page=2&limit=10 to have page 2 when the number of records on each page must be 10

This feature is enabled for these end-points:

/stock/lots/depots/ : for getting the list of lots with their quantities (see Lots in stock registry)
/stock/lots/depotsDetailed/ : for getting the detailed list of lots (see Lots in stock registry)
/stock/lots/movements/ : for getting the list of stock movements made (see Stock movements registry)
/stock/inventories/depots/ : for getting the list of inventories without consideration of lots (see Article in stock registry)

jmcameron · 2023-03-06T19:44:45Z

Please add a description of how to test this PR.

jniles · 2023-03-06T22:54:21Z

I'm also curious what this is going to be used for. It seems like it might be better to create it's own route. I think all API routes in BHIMA that support filtering currently return the maximum amount of records that meet a user's filter criteria, and this breaks that API contract.

mbayopanda · 2023-03-07T04:46:03Z

I'm also curious what this is going to be used for. It seems like it might be better to create it's own route. I think all API routes in BHIMA that support filtering currently return the maximum amount of records that meet a user's filter criteria, and this breaks that API contract.

@jniles this can be used for server-side pagination, which gives valuable information to the user instead of just loading a limited number of records to not break the client, the pagination can be applied to any list of records (grid, table, etc.) or through API call directly.

Actually, the BHIMA API is huge, so we cannot apply this change to all routes but we can do that progressively without breaking the current behavior of BHIMA routes.

jmcameron

The code looks okay. I tried the suggested tests and they seem to work fine.

It is hard to evaluate the utility of this change until it is applied to a report or register page, so I do not have a firm opinion on whether this is a good idea or not. But I do not object to merging it since I think (guess) it will not affect any existing pages or reports.

jmcameron · 2023-03-15T15:11:45Z

@mbayopanda What do you want to do next on this PR? Do we need further discussion?

jmcameron · 2023-03-16T15:00:30Z

According to @mbayopanda (on WhatsApp): Currently there is the BAO System team for their collaboration in the SEMI project they would like to integrate the BHIMA data into their report; they have been using our current API and they have asked me if we can provide the data per page, which will allow not loading all the data at once but as needed. We can merge this PR as long as it doesn't break what's already there.

jmcameron

I looked at the code again and it seems fine.
I tested it with the implemented endpoints and they work nicely.

I had one question that you might consider: What should the REST queries return for pages beyond the end of the data? According to some best practices perhaps we should return a HTML 204 response. Currently a empty JSON "pager" object is returned. For instance, calling

http://localhost:8080/stock/lots/depots/?paging=true&limit=4&page=4

Produces

{"pager":{"total":18,"page":4,"page_size":4,"page_min":12, "page_max":16,"page_count":5},"rows":[]}

It is not clear that the receiving party will be able to interpret this correctly.
Notice that there is no indication that there are only 3 pages with data. Also, what does the "page_max" mean here? There are 11 items total. Perhaps we should discuss this with the BAO folks and see what response would work best for them.

mbayopanda · 2023-03-20T06:16:35Z

I looked at the code again and it seems fine. I tested it with the implemented endpoints and they work nicely.

I had one question that you might consider: What should the REST queries return for pages beyond the end of the data? According to some best practices perhaps we should return a HTML 204 response. Currently a empty JSON "pager" object is returned. For instance, calling

http://localhost:8080/stock/lots/depots/?paging=true&limit=4&page=4

Produces

{"pager":{"total":18,"page":4,"page_size":4,"page_min":12, "page_max":16,"page_count":5},"rows":[]}

It is not clear that the receiving party will be able to interpret this correctly. Notice that there is no indication that there are only 3 pages with data. Also, what does the "page_max" mean here? There are 11 items total. Perhaps we should discuss this with the BAO folks and see what response would work best for them.

@jmcameron this is a bug with the way we use SELECT COUNT(*) we have to use SELECT COUNT(DISTINCT specific_column) since the code is used for many context in this context the result is not correct;

A temporary solution is to use naive code and javascript to fix that.

mbayopanda · 2023-03-21T08:45:31Z

@jmcameron could you review this PR please

jmcameron

I reviewed the code again and made some suggestions for improving variable names, etc.

I tested the end points using bhima_test:

/stock/lots/depots/?paging=true worked as expected
/stock/lots/depotsDetailed/ failed with this message: {"status":500}
/stock/lots/movements/?paging=true worked as expected
/stock/inventories/depots/ worked as expected, but when I asked for a page beyond the limit, I got: [] which is a different response than the other endpoints.

Suggestions

All endpoints should return data in the same format
When asking for a page beyond the limit, some endpoints return:
- {"rows":[],"pager":{"total":18,"page":3,"page_size":10,"page_min":20, "page_max":30,"page_count":2}}
- I see that page_count has been corrected. Thank you.
- However, I would prefer to see page_min and page_max to be null or something to indicate that they are not valid for this page beyond the end of the data. I'm happy to see that rows = [] in this case, but I think that it would be clearer to whoever is dealing with this data to know that the page_min and page_max values are invalid in this response.

jmcameron · 2023-03-21T15:30:01Z

server/controllers/stock/core.js


  // add the minimum delay to the rows
-  filteredRows.forEach(row => {
+  _filteredRows.forEach(row => {


I find using the _ prefix not very helpful for making the code easy to understand. Rather than _filteredRows, I suggest using filteredRowsPaged or filteredRowsPaging (or something similar) which is more descriptive and easy to understand why _filteredRows is different from filteredRows.

jmcameron · 2023-03-21T15:30:58Z

server/controllers/stock/index.js

@@ -1169,18 +1173,21 @@ async function listLotsDepotDetailed(req, res, next) {
      db.exec(sqlGetMonthlyStockMovements, [db.bid(params.depot_uuid), params.startDate, params.dateTo]),
    ]);

-    data.forEach(current => {
+    const _data = !params.paging ? data : data.rows;
+    const _dataPreviousMonth = !params.paging ? dataPreviousMonth : dataPreviousMonth.rows;


Again, I suggest calling this dataPreviousMonthPaged or something similar to improve code readability.

jmcameron · 2023-03-21T15:31:31Z

server/controllers/stock/index.js

@@ -1169,18 +1173,21 @@ async function listLotsDepotDetailed(req, res, next) {
      db.exec(sqlGetMonthlyStockMovements, [db.bid(params.depot_uuid), params.startDate, params.dateTo]),
    ]);

-    data.forEach(current => {
+    const _data = !params.paging ? data : data.rows;


I suggest dataPaged

jmcameron · 2023-03-21T15:35:26Z

server/lib/db/index.js

+      // FIXME: Performance issue, use SQL COUNT in a better way
+      const total = (await this.exec(filters.getAllResultQuery(sql.concat(' ', tables)), queryParameters)).length;
+      const page = params.page ? parseInt(params.page, 10) : 1;
+      const limit = params.limit ? parseInt(params.limit, 10) : 100;


I suggest computing page_count here. And when computing page_min and page_max below, if page > page_count, set them to `null' since they are invalid for pages beyond the max. And apply similar fixes to the FilterParser below.

We have to set page_min and page_max after the execution of the query since the page_min is sent to the SQL query in the OFFSET part.

mbayopanda · 2023-03-26T14:24:01Z

I reviewed the code again and made some suggestions for improving variable names, etc.

I tested the end points using bhima_test:

/stock/lots/depots/?paging=true worked as expected

/stock/lots/depotsDetailed/ failed with this message: {"status":500}

/stock/lots/movements/?paging=true worked as expected

/stock/inventories/depots/ worked as expected, but when I asked for a page beyond the limit, I got: [] which is a different response than the other endpoints.

Suggestions

All endpoints should return data in the same format

When asking for a page beyond the limit, some endpoints return:

{"rows":[],"pager":{"total":18,"page":3,"page_size":10,"page_min":20, "page_max":30,"page_count":2}}

I see that page_count has been corrected. Thank you.

However, I would prefer to see page_min and page_max to be null or something to indicate that they are not valid for this page beyond the end of the data. I'm happy to see that rows = [] in this case, but I think that it would be clearer to whoever is dealing with this data to know that the page_min and page_max values are invalid in this response.

Thank you @jmcameron for your review

mbayopanda · 2023-03-28T09:39:29Z

@jmcameron can I get a review, please?

About this /stock/lots/depotsDetailed/ failed with this message: {"status":500}, the end-point requires a depot_uuid in params, it's a wrong definition of a REST endpoint.

jmcameron

Thanks for the code improvements.

I tested the endpoints again:

/stock/lots/depots/?paging=true worked as expected
/stock/lots/movements/?paging=true worked as expected
/stock/inventories/depots/?paging=true worked as expected
/stock/lots/depotsDetailed/ seems to be working(?)

Regarding depotsDetailed. When I use the URL below, I get no results, which I do not understand, since the UUID if for the primary depot. It is likely that I have the format or data of the URL incorrect.

/stock/lots/depotDetailed/?depot_uuid=F9CAEB16168443C5A6C447DBAC1DF296
Produces: []

However, when I use paging:

/stock/lots/depotDetailed/?depot_uuid=F9CAEB16168443C5A6C447DBAC1DF296&paging=true
Produces: {"pager":{"total":0,"page":1,"page_size":100,"page_min":null,"page_max":null,"page_count":0},"rows":[]}
Which looks like the paging part is working correctly

As far as I'm concerned, I think this is ready to go.

jmcameron · 2023-03-28T15:28:38Z

server/controllers/stock/core.js

-  let filteredRows = await getLots(sql, params, clause);
-  if (filteredRows.length === 0) { return []; }
+  const filteredRows = await getLots(sql, params, clause);
+  let filteredRowsPaged = params.paging ? filteredRows.rows : filteredRows;


jmcameron · 2023-03-28T15:31:18Z

server/lib/db/index.js

+      if (rows.length === 0) {
+        // update page_min and page_max after the query
+        // in case of empty result
+        pager.page_min = null;


mbayopanda · 2023-04-01T14:19:08Z

Thank you @jmcameron

bors r+

bors · 2023-04-01T14:22:12Z

Build succeeded:

ci/semaphoreci/push: Integration Tests

jmcameron reviewed Mar 7, 2023

View reviewed changes

mbayopanda force-pushed the paging-feature branch from b75b2ed to aa4f14f Compare March 15, 2023 08:31

jmcameron reviewed Mar 16, 2023

View reviewed changes

mbayopanda force-pushed the paging-feature branch from aa4f14f to 7c5c7d0 Compare March 20, 2023 06:22

jmcameron reviewed Mar 21, 2023

View reviewed changes

mbayopanda added 5 commits March 28, 2023 09:44

Add pagination in the stock_movement end points

f26c3e2

paginate /stock/lots/depots

4c27665

Add pagination for stock end-points

d5e17e2

fix pager.total value for multiple context

c65c68c

fix typo and harmonize output

3f87b8d

mbayopanda force-pushed the paging-feature branch from 7c5c7d0 to 3f87b8d Compare March 28, 2023 09:37

jmcameron reviewed Mar 28, 2023

View reviewed changes

bors bot merged commit 10076d9 into Third-Culture-Software:master Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pagination in the stock_movement end-point #6960

Add pagination in the stock_movement end-point #6960

mbayopanda commented Mar 6, 2023 •

edited

Loading

jmcameron commented Mar 6, 2023

jniles commented Mar 6, 2023

mbayopanda commented Mar 7, 2023

jmcameron left a comment •

edited

Loading

jmcameron commented Mar 15, 2023

jmcameron commented Mar 16, 2023

jmcameron left a comment

mbayopanda commented Mar 20, 2023 •

edited

Loading

mbayopanda commented Mar 21, 2023

jmcameron left a comment

jmcameron Mar 21, 2023

jmcameron Mar 21, 2023

jmcameron Mar 21, 2023

jmcameron Mar 21, 2023

mbayopanda Mar 28, 2023

mbayopanda commented Mar 26, 2023

mbayopanda commented Mar 28, 2023

jmcameron left a comment •

edited

Loading

jmcameron Mar 28, 2023

jmcameron Mar 28, 2023

mbayopanda commented Apr 1, 2023

bors bot commented Apr 1, 2023

Add pagination in the stock_movement end-point #6960

Add pagination in the stock_movement end-point #6960

Conversation

mbayopanda commented Mar 6, 2023 • edited Loading

jmcameron commented Mar 6, 2023

jniles commented Mar 6, 2023

mbayopanda commented Mar 7, 2023

jmcameron left a comment • edited Loading

Choose a reason for hiding this comment

jmcameron commented Mar 15, 2023

jmcameron commented Mar 16, 2023

jmcameron left a comment

Choose a reason for hiding this comment

mbayopanda commented Mar 20, 2023 • edited Loading

mbayopanda commented Mar 21, 2023

jmcameron left a comment

Choose a reason for hiding this comment

jmcameron Mar 21, 2023

Choose a reason for hiding this comment

jmcameron Mar 21, 2023

Choose a reason for hiding this comment

jmcameron Mar 21, 2023

Choose a reason for hiding this comment

jmcameron Mar 21, 2023

Choose a reason for hiding this comment

mbayopanda Mar 28, 2023

Choose a reason for hiding this comment

mbayopanda commented Mar 26, 2023

mbayopanda commented Mar 28, 2023

jmcameron left a comment • edited Loading

Choose a reason for hiding this comment

jmcameron Mar 28, 2023

Choose a reason for hiding this comment

jmcameron Mar 28, 2023

Choose a reason for hiding this comment

mbayopanda commented Apr 1, 2023

bors bot commented Apr 1, 2023

mbayopanda commented Mar 6, 2023 •

edited

Loading

jmcameron left a comment •

edited

Loading

mbayopanda commented Mar 20, 2023 •

edited

Loading

jmcameron left a comment •

edited

Loading